[client] Add retry based on the httpStatusErrorCode in endStreamAction #771
Conversation
| s"with error message ${lastEndStreamAction.errorMessage}") | ||
| throw new DeltaSharingServerException( | ||
| s"Server Exception: ${lastEndStreamAction.errorMessage}", | ||
| Option(lastEndStreamAction.httpStatusErrorCode).map(_.intValue)) |
There was a problem hiding this comment.
nit question: is it better to put error code in front?
There was a problem hiding this comment.
Do you mean in the error message? Would it be useful?
There was a problem hiding this comment.
To me, errorCode is more abstracted and quick glance of what's happening.
Also, do we know what's the user facing error with this new definition of DeltaSharingServerException? do they see the errorCode? it could also be helpful for debugging.
There was a problem hiding this comment.
Right now, no I don't think the errorCode will be showing in the error message since we are not overriding any of the default converters from the RuntimeException class.
I pushed a change to pre-pend the error message with the status code.
There was a problem hiding this comment.
nit: L1372 can use errorCodeOpt
There was a problem hiding this comment.
And also nit UX: Does "Server Exception500" look good?
| s"with error message ${lastEndStreamAction.errorMessage}") | ||
| throw new DeltaSharingServerException( | ||
| s"Server Exception: ${lastEndStreamAction.errorMessage}", | ||
| Option(lastEndStreamAction.httpStatusErrorCode).map(_.intValue)) |
There was a problem hiding this comment.
nit: L1372 can use errorCodeOpt
| s"with error message ${lastEndStreamAction.errorMessage}") | ||
| throw new DeltaSharingServerException( | ||
| s"Server Exception: ${lastEndStreamAction.errorMessage}", | ||
| Option(lastEndStreamAction.httpStatusErrorCode).map(_.intValue)) |
There was a problem hiding this comment.
And also nit UX: Does "Server Exception500" look good?
There was a problem hiding this comment.
The change itself looks good.
After this code change, getFile could run longer than before. In a streaming query, it’s possible to have multiple getBatch calls to DeltaSharingDatasource. I’m wondering whether we need to make maybeGetFileChanges thread-safe within a datasource.
@linzhou-db
| s"with error message ${lastEndStreamAction.errorMessage}") | ||
| val errorCodeOpt = Option(lastEndStreamAction.httpStatusErrorCode).map(_.intValue) | ||
| throw new DeltaSharingServerException( | ||
| s"Server Exception${errorCodeOpt.getOrElse("")}: " + |
There was a problem hiding this comment.
Server Exception[${...}]?
Hmmm, good point, what's the consequence if not? @littlegrasscao |
I guess these issues could happen today if multiple getBatch calls occur and queryTable from version a to b runs for a long time. So far, we haven’t seen any issues, right? |
4 participants
In this PR
httpStatusErrorCodeinEndStreamActionin both client/model and server/model.We already retry based on the http status code of the http request but not based on the status code of errors that happen after the header/status code has been sent. This retry is being added here. The way that it is being implemented is by making both the cases extend the same base exception class
DeltaSharingExceptionWithErrorCode.Added unit tests for the changes.